[MWAR-443] Fix deletion of files placed by maven-dependency-plugin - #641
[MWAR-443] Fix deletion of files placed by maven-dependency-plugin#641elharo wants to merge 1 commit into
Conversation
elharo
changed the title
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes MWAR-443 by preventing the WAR plugin from deleting WEB-INF/lib files that were placed by other plugins (e.g., maven-dependency-plugin) and aren’t runtime-scope dependencies managed by the WAR plugin.
Changes:
- Add runtime-artifact filename computation and use it to gate “outdated” marking under
WEB-INF/lib/. - Add a new unit test ensuring a “provided-scope-like” jar in
WEB-INF/libis preserved duringexplodedexecution.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/main/java/org/apache/maven/plugins/war/AbstractWarMojo.java | Avoids marking non-runtime WEB-INF/lib files as outdated by matching only runtime artifact filenames. |
| src/test/java/org/apache/maven/plugins/war/WarExplodedMojoTest.java | Adds regression test for MWAR-443 to ensure dependency-plugin-provided jars are not deleted. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void testProvidedScopeArtifactPlacedByDependencyPluginShouldNotBeDeleted(WarExplodedMojo mojo) | ||
| throws Exception { | ||
| // Ensure session has a start time so the outdated resource detection is active | ||
| when(mavenSession.getStartTime()).thenReturn(new Date()); |
There was a problem hiding this comment.
This follows the same pattern used by WarExplodedMojoFilteringTest (which calls when(mavenSession.getSystemProperties()).thenReturn(...)). The test harness provides a Mockito mock for @Inject MavenSession. The test passes successfully with this approach.
| File webAppDirectory = mojo.getWebappDirectory(); | ||
| File libDir = new File(webAppDirectory, "WEB-INF/lib"); | ||
| libDir.mkdirs(); | ||
| File providedJar = new File(libDir, "derbyLocale_cs-10.14.2.0.jar"); | ||
| providedJar.createNewFile(); | ||
| // Set timestamp to something in the past (before session start) | ||
| providedJar.setLastModified(0L); | ||
|
|
||
| // Run the mojo | ||
| mojo.execute(); | ||
|
|
||
| // The file should NOT be deleted - it was placed by another plugin | ||
| assertTrue(providedJar.exists(), "provided-scope artifact should not be deleted by WAR plugin"); | ||
|
|
||
| // Cleanup | ||
| providedJar.delete(); | ||
| libDir.delete(); | ||
| } |
There was a problem hiding this comment.
Fixed in the latest revision. Cleanup is now in a finally block with proper resource cleanup.
| String classifier = artifact.getClassifier(); | ||
| if (classifier != null && !classifier.trim().isEmpty()) { | ||
| fileNames.add(MappingUtils.evaluateFileNameMapping( | ||
| MappingUtils.DEFAULT_FILE_NAME_MAPPING_CLASSIFIER, artifact)); | ||
| } else { | ||
| fileNames.add(MappingUtils.evaluateFileNameMapping( | ||
| MappingUtils.DEFAULT_FILE_NAME_MAPPING, artifact)); | ||
| } |
There was a problem hiding this comment.
Addressed in the latest revision. getRuntimeArtifactFileNames() now checks getOutputFileNameMapping() and uses it if set, mirroring the same logic as ArtifactsPackagingTask.getArtifactFinalName().
| String type = artifact.getType(); | ||
| if ("jar".equals(type) | ||
| || "ejb".equals(type) | ||
| || "ejb-client".equals(type) | ||
| || "test-jar".equals(type) | ||
| || "bundle".equals(type) | ||
| || "par".equals(type)) { |
There was a problem hiding this comment.
Addressed in the latest revision. Extracted isLibraryType() to AbstractWarPackagingTask and now use it from both ArtifactsPackagingTask.performPackaging() and getRuntimeArtifactFileNames(), eliminating the duplication.
Fixes apache#522 Address code review comments: - Extract shared isLibraryType() in AbstractWarPackagingTask to avoid duplicating artifact type list across the codebase - Use outputFileNameMapping in getRuntimeArtifactFileNames() when set - Use isLibraryType() in both ArtifactsPackagingTask and the outdated resource detection logic - Fix test cleanup with try/finally and recursive delete
Fixes #522
The WAR plugin's outdated resource detection was incorrectly marking files in
WEB-INF/libas outdated even when they were placed by other plugins (e.g.,maven-dependency-plugin) for non-runtime-scope artifacts. These files would then be deleted bydeleteOutdatedResources().The fix computes the expected filenames for runtime-scope artifacts and only marks files under
WEB-INF/libas outdated if they match one of those filenames. Files placed by other plugins for provided-scope or other non-runtime artifacts are no longer touched.Root cause: When the
DefaultWarPackagingContextconstructor walks thewebappDirectory, it marks files older thansession.getStartTime()as "outdated". Files copied bymaven-dependency-plugintoWEB-INF/lib/have timestamps from the local Maven repository (old), so they get marked as outdated. The WAR plugin only copies runtime-scope artifacts, soprovided-scope artifacts are never "claimed" viaaddResource(), and are subsequently deleted.Fix: Before marking a file under
WEB-INF/lib/as outdated, verify it matches a runtime-scope artifact filename. If it doesn't, it was placed by another plugin and should be preserved.